-
-
Notifications
You must be signed in to change notification settings - Fork 94
AI GENERATED NEEDS REVIEW: convert reflector to cljc #1007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Completely generated by copilot agent (Claude Sonnet 4.5) Co-authored-by: copilot-swe-agent[bot] <[email protected]>
| (when context-class (str " for " context-class))))) | ||
| (let [^Method m (if (= 1 (count methods)) | ||
| (first methods) | ||
| (or (match-method methods args arg-types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (or (match-method methods args arg-types) | |
| ;; overloaded w/same arity | |
| (or (match-method methods args arg-types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think this is a problem? It calls match-method with a widened box arg?
| (first methods) | ||
| (or (match-method methods args arg-types) | ||
| ;; widen boxed args and re-try | ||
| (match-method methods (widen-boxed-args args) arg-types)))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG! Original code updates args for use later in the method:
args = widenBoxedArgs(args);
m = matchMethod(methods, args, argTypes);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually isn't a bug since widen-boxed-args also updates the args and returns it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, now I see it. It does create a new array and then it sets the args for the next portion of the method... BUG!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this will really result in a bug though. It'd be interested to extract a test case for this one to see where it currently fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the args are widened to find the correct method, e.g. Double -> whatever instead of Float -> whatever. But then later the Float argument is boxed and passed to the method which may just work...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example that this will just work out ok I think:
(ns reflect-example
(:import [java.lang Math]
[java.lang.reflect Method]))
;; Get the Method object for Math.pow(double, double)
(def m (.getMethod Math "pow" (into-array Class [Double/TYPE Double/TYPE])))
;; Float value
(def f (float 3.0))
(prn (type f))
;; Attempt to invoke with Float (boxed) arguments
(try
(.invoke m nil (into-array Object [f (float 1.0)]))
(catch IllegalArgumentException e
(println "Caught exception:" (.getMessage e))))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we can see that it works in bb:
$ ./bb -e '(Math/pow (float 1.33) (float 2.0))'
1.7689001141548175
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll ask in clojure-dev. We could make widen-box-args mutate the original array if we want re-assigment.
MOVED TO https://github.com/babashka/sci/tree/frenchy64-cljc-reflector
Completely generated by copilot agent (Claude Sonnet 4.5)
Please answer the following questions and leave the below in as part of your PR.
I have read the developer documentation.
This PR corresponds to an issue with a clear problem statement.
This PR contains a test to prevent against future regressions